Add yihui_style()#449
Conversation
yihui
left a comment
There was a problem hiding this comment.
Haha, I just hope this function is not named after me or any particular person 😁
Perhaps a more explicit name like eq_assign_style()?
|
It's called the Yihuiverse style guide. It's official now. haha. No, but it was already called kind of Yihui's style guide in #340 (comment) 😄. Of course, we can also go for a different name if @yihui does not agree and I think it could be more informative too. |
|
Hehe I like the
I can think of various +s and -s of each name. I'd be happy with any of them and there may be more viable options I've not seen/thought of. I think you guys (primarily @lorenzwalthert and @yihui ) should have a greater say than me over the name so up to you: I'll happily change the name in the PR. Also happy to add tests and make any further changes / put the code elsewhere pending feedback. Regarding whether
I'm not sure. Are there any other changes I should make for this to be 'yihuiverse-compliant' ✔️ @yihui hehe? In summary: thanks for the feedback; I'll make changes pending more feedback, particular relating to the style's name. |
yihui
left a comment
There was a problem hiding this comment.
I'm fine with kidding, but please don't make "Yihui style" or "Yihuiverse" official. I consider my style a personal taste, and I don't intend to promote it. Other than the style name, I don't have further comments.
Sure. I think we better find another name for the style guide that does not involve your name. |
Codecov Report
@@ Coverage Diff @@
## master #449 +/- ##
==========================================
- Coverage 91.02% 90.55% -0.47%
==========================================
Files 36 36
Lines 1626 1631 +5
==========================================
- Hits 1480 1477 -3
- Misses 146 154 +8
Continue to review full report at Codecov.
|
|
Thanks for the feedback @yihui I've updated the style name in-line with my original thinking - used |
|
Another issue with this PR I've discovered is that the style does not work as expected: txt = "x = c(
1
)"
styler::style_text(txt)
#> x <- c(
#> 1
#> )
styler::style_text(txt, style = styler::equals_style)
#> x = c(
#> 1
#> )Created on 2018-12-05 by the reprex package (v0.2.1) @lorenzwalthert any suggestions on how to fix this? At present I'm setting the tokens as follows - was thinking of naming the list but not sure if that will help. Here's the function at present in any case: equals_style <- function() {
create_style_guide(token = list(force_assignment_op_equals))
} |
|
Regarding tests, I've had a first bash but not sure if this fits with your previous tests. Advice on this also appreciated @lorenzwalthert: library(styler)
library(testthat)
txt_left_assign = "x <- 1"
txt_equals_assign = "x = 1"
res_equals_style = style_text(txt_left_assign, style = equals_style)
txt_equals_style = as.character(res_equals_style)
test_that("equals_style replaces <- with = in one-liner",{
expect_identical(
txt_equals_style,
txt_equals_assign)
})Created on 2018-12-05 by the reprex package (v0.2.1) |
|
As far as tests go, We have a testing framework powered by As far as the indention problem goes we need library(styler)
force_assignment_op_equals <- function(pd) {
to_replace <- pd$token == "LEFT_ASSIGN"
pd$token[to_replace] <- "EQ_ASSIGN"
pd$text[to_replace] <- "="
pd
}
equals_style <- function() {
create_style_guide(
token = list(force_assignment_op_equals),
use_raw_indention = TRUE
)
}
txt <- "x = c(
1
)"
style_text(txt, style = equals_style)
#> x = c(
#> 1
#> )Created on 2018-12-05 by the reprex package (v0.2.1) |
|
I also updated |
|
Good news on the testing front: Some of those examples could be good in the tests I imagine. |
- check on data.tree version, not R version (r-lib#450).
|
See my comments in the repo there. Maybe there is an unexpected interaction with #381. Also, you could rebase on master to make the (unrelated) CI failure on R 3.2 go away now. |
1 similar comment
|
See my comments in the repo there. Maybe there is an unexpected interaction with #381. Also, you could rebase on master to make the (unrelated) CI failure on R 3.2 go away now. |
|
Think I've borked the rebase. Can create a new clean PR if that would make sense - suggested way forward unless there are better suggestions. |
A little context: ropensci/software-review-meta#71 (comment)